Clone kafka 18894 #33
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughConfig providers are refactored to be handled as Plugin-wrapped instances across core config, Connect runtime, and MirrorMaker. Constructors, fields, and method signatures are updated accordingly. Metrics support is integrated for monitorable providers. Tests are adjusted to use Plugin wrappers and include new coverage for monitorable provider metrics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant AC as AbstractConfig
participant CT as ConfigTransformer
participant PP as ProviderPlugins (Map<String, Plugin<ConfigProvider>>)
participant CP as ConfigProvider
App->>AC: resolveConfigVariables(configs)
AC->>AC: instantiateConfigProviders() -> PP
AC->>CT: new ConfigTransformer(PP)
App->>CT: transform(configs)
CT->>PP: lookup providerPlugin by name
alt provider found
CT->>PP: providerPlugin.get()
PP-->>CT: CP
CT->>CP: get(path, keys)
CP-->>CT: ConfigData
CT-->>App: transformed configs
else provider missing
CT-->>App: unchanged or error per logic
end
AC->>PP: closeQuietly each provider plugin
sequenceDiagram
autonumber
actor Admin as Connect Worker Startup
participant W as Worker
participant Pl as Plugins
participant M as Metrics
participant PPl as Plugin<ConfigProvider>
participant WCT as WorkerConfigTransformer
Admin->>W: start()
W->>Pl: newConfigProvider(config, providerName, PLUGINS, M)
Pl->>Pl: compute providerPrefix
Pl->>Pl: instantiate ConfigProvider
Pl->>PPl: Plugin.wrapInstance(provider, metadata:{provider})
Pl-->>W: PPl
W->>WCT: new WorkerConfigTransformer(W, {name -> PPl})
note over WCT: Transformer holds Plugin-wrapped providers
Admin-->>W: running
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.5)connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The user @DDShantanuBadmanji does not have an active seat allocated for code reviews. Please contact your organization admin to assign a seat or upgrade your subscription. |
Summary of ChangesHello @DDShantanuBadmanji, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a foundational change to how ConfigProvider instances are handled within the Kafka ecosystem. By wrapping ConfigProviders in a generic Plugin object, the system gains improved capabilities for standardized lifecycle management and enables automatic metrics registration for providers that implement the Monitorable interface. This enhancement aims to provide better observability and a more robust plugin architecture for configuration management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/refacto-test |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of ConfigProvider to introduce a Plugin wrapper, enabling metrics for monitorable providers. The changes are well-implemented across the client and connect modules, with corresponding updates to tests. I've found one area for improvement regarding redundant configuration transformation in MirrorMakerConfig, which could be simplified for better efficiency. Overall, this is a good enhancement.
| Map<String, String> transform(Map<String, String> props) { | ||
| // transform worker config according to config.providers | ||
| List<String> providerNames = configProviders(); | ||
| Map<String, ConfigProvider> providers = new HashMap<>(); | ||
| Map<String, Plugin<ConfigProvider>> providerPlugins = new HashMap<>(); | ||
| for (String name : providerNames) { | ||
| ConfigProvider configProvider = plugins.newConfigProvider( | ||
| Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider( | ||
| this, | ||
| CONFIG_PROVIDERS_CONFIG + "." + name, | ||
| Plugins.ClassLoaderUsage.PLUGINS | ||
| name, | ||
| Plugins.ClassLoaderUsage.PLUGINS, | ||
| null | ||
| ); | ||
| providers.put(name, configProvider); | ||
| providerPlugins.put(name, configProviderPlugin); | ||
| } | ||
| ConfigTransformer transformer = new ConfigTransformer(providers); | ||
| ConfigTransformer transformer = new ConfigTransformer(providerPlugins); | ||
| Map<String, String> transformed = transformer.transform(props).data(); | ||
| providers.values().forEach(x -> Utils.closeQuietly(x, "config provider")); | ||
| providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin")); | ||
| return transformed; | ||
| } |
There was a problem hiding this comment.
This transform method appears to be redundant and leads to inefficiency. Config providers are instantiated, used, and closed here, but the configurations produced by clientConfig and workerConfig are later passed to AbstractConfig subclasses (MirrorClientConfig and WorkerConfig respectively), which perform the same transformation step again.
This means config providers are initialized twice. The second transformation will likely be a no-op on already-resolved values, but the setup and teardown of providers is unnecessary overhead.
It would be more efficient to remove this transform method and the calls to it in clientConfig and workerConfig. AbstractConfig's constructor will handle the variable resolution automatically.
|
The user @DDShantanuBadmanji does not have an active seat allocated for code reviews. Please contact your organization admin to assign a seat or upgrade your subscription. |
2 participants
Delete this text and replace it with a detailed description of your change. The
PR title and body will become the squashed commit message.
If you would like to tag individuals, add some commentary, upload images, or
include other supplemental information that should not be part of the eventual
commit message, please use a separate comment.
If applicable, please include a summary of the testing strategy (including
rationale) for the proposed change. Unit and/or integration tests are expected
for any behavior change and system tests should be considered for larger
changes.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores